r? @kennytm

(rust-highfive has picked a reviewer for you, use r? to override)

This is a follow-on from #82411. See also #82974 which I have just submitted.

☔ The latest upstream changes (presumably #82982) made this pull request unmergeable. Please resolve the merge conflicts.

cc @yaahc for review regarding the handling of exit statuses as errors, and potential interaction with Termination and other error-handling proposals.

cc @yaahc for review regarding the handling of exit statuses as errors, and potential interaction with Termination and other error-handling proposals.

I considered the potential interactions with several areas of process exit status handling. A review is a good idea, but it might be helpful to set out my thoughs:

Termination is the return value from main. I think the docs for process::ExitStatus (in Nightly) and for ExitStatusError (in this MR) already properly relate the return value from ExitStatus[Error]::code() to the the value returned from main. Introducing ExitStatusError doesn't change that. So I don't think there is any nontrivial interaction with Termination.

There is also std::process::ExitCode (unstable). Here is more of a can of worms.

To discuss this clearly means dealing with a terminological problem. On Unix there is a "wait status" (as you get from wait* system calls) and an "exit status" (as you pass to exit). A wait status might represent an exit status, but it might alternatively represent some other kind of process termination (or, more generally, state).

Rust stdlib uses the term "exit code" (or just code) for what Unix calls "exit status"; and it uses "exit status" for what Unix calls "wait status". So a Rust "exit status" can be an "exit code" or some other kind of process termination.

I am going to use the term "exit value integer" for the value passed to exit.

One problem is that Rust stdlib thinks an exit value integer is i32. But in C it is an int. But but on Unix only the bottom 8 bits are used. This leads to a number of strangenesses.

For example, on Fuchsia (AFAICT) the actual exit value integer is a u64. But ExitStatus::code must return an i32 and is therefore broken.

Another example: on Unix you can pass a value >255 to std::process::exit or return it from main, but only the bottom bits are preserved: observe that here https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=de0af65b4d80dedc8b8c4934c353bd90 we appear to see success! This is a footgun.

ExitCode might be part of the solution to these problems, by generalising the exit value integer. But the other methods in process, and the integer return value type of main and in Termination, are all i32.

My present MR does not attempt to drain this swamp. I think it could only be drained by replacing many of the affected methods in process, and maybe changing the allowable return types from main (!) There are many problems with process::Command, so it might be a good idea to replace process in its entirety anyway.

I think that my addition of ExitStatusError does not make this any worse, at least. Whatever solution is adopted for ExitStatus::code() can be adopted for ExitStatusError::code().

I'm not suggesting this PR needs to solve this problem. Rather, there are specific proposals in development to allow attaching information to errors, such as the numeric value to exit the process with, or am HTTP response code. I'm asking for @yaahc's review to see if this potentially interacts with any of those proposals.

cc @yaahc for review regarding the handling of exit statuses as errors, and potential interaction with Termination and other error-handling proposals.

reviewing, also cc @JDuchniewicz who is currently leading the effort on stabilizing Termination.

edit: still reviewing, but this seems for the most part separate from Termination so far, so I'm assuming this wouldn't conflict with the generic member access RFC. The only way they interact is that we'd probably want to have ExitStatusError use provide to pass it's ExitCode out too the Termination impl, though I just learned some bad news related to specialization so I'm not as positive that the Termination plans we had are doable.

Hi. I think all the comments so far have been addressed. I would like to get this merged because it's blocking fixing almost all the examples for Command. Currently those examples generally fail to check the exit status at all, and several of them leak the child process.

@yaahc I think you are happy that this doesn't cause trouble for the stabilisation of Termination. Do you want to wait for a double-check from @JDuchniewicz? Apart from that, I think this is then just in need of a review by a T-libs member.

If that review is positive, I'll open a tracking issue, update the MR (and probably rebase...), etc.

Thanks.

@yaahc I think you are happy that this doesn't cause trouble for the stabilisation of Termination. Do you want to wait for a double-check from @JDuchniewicz? Apart from that, I think this is then just in need of a review by a T-libs member.

I'm quite confident there won't be any issues here with respect to the Termination trait.

If that review is positive, I'll open a tracking issue, update the MR (and probably rebase...), etc.

I've gone ahead and done another review pass with a few more comments. Once those get resolved I think we can either go ahead and merge this, or, if there are still changes to the stable API of std in the PR, we will need to start an FCP.

Read through the PR and it looks good to me. As @yaahc mentioned, it will allow us for more options for reporting the error/exit status inside the Termination trait.

@yaahc Thanks for the review. I think I have addressed your comments. I also opened a tracking issue and updated the #[unstable] attributes with it.

For now I haven't squashed this down to assist re-review. Let me know when I should do that (and rebase onto current trunk).

☔ The latest upstream changes (presumably #84200) made this pull request unmergeable. Please resolve the merge conflicts.

@yaahc Thanks for the review. I have fixed the WASM build, I think - it compiles locally, anyway. I also checked the entries in library/std/src/sys/mod.rs and they all seem to go to unsupported so I hope it should be good everywhere now.

I also took the opportunity to autosquash the fixmes into the underlying commits. I didn't rebase, so the "force pushed" message from github (or plain git diff fb2db8d..26c782b) has just the diff for the WASM fix.

@yaahc Would you mind giving this your (re)review and approval?

@bors r+

📌 Commit 26c782b has been approved by yaahc

⌛ Testing commit 26c782b with merge 25a277f...

☀️ Test successful - checks-actions
Approved by: yaahc
Pushing 25a277f to master...

Performance triage indicates that this PR introduced a 1.7% regression when fully compiling coercions-debug.

I don't think any performance hit was expected. However, it also seems like this may be just noise.

Performance triage indicates that this PR introduced a 1.7% regression when fully compiling coercions-debug.

How strange. Can you perhaps give me a reference to what "coercions-debug" is? I searched rust-lang/rust for it, and asked a general search engine, and I looked for links in the perf report you linked to, but none of those approaches was fruitful.

I don't think any performance hit was expected. However, it also seems like this may be just noise.

Definitely, a performance hit was not expected. I see the result is annotated with a single ? and the text says "measurements known to have high variation are marked with '?'/'??'". The linkified "noise run" seems to have 0 variation in this test.

@ijackson yeah we are actually discussing the noise associated with this specific benchmark in Zulip now (zulip archive, zulip live)

The source code to coercions is here: https://github.com/rust-lang/rustc-perf/blob/master/collector/benchmarks/coercions/src/main.rs

It is definitely a micro-benchmark that is not representative of real code.

(And, again, to be clear, We think we are just seeing noise here. Not something actually problematic in your PR.)

The source code to coercions is here: https://github.com/rust-lang/rustc-perf/blob/master/collector/benchmarks/coercions/src/main.rs

It is definitely a micro-benchmark that is not representative of real code.

Thanks for the link. So I see!

(And, again, to be clear, We think we are just seeing noise here. Not something actually problematic in your PR.)

Right, OK, thanks. Happy hunting. (I have bookmarked that paper by Kalibera & Jones since it piqued my curiosity)